Fix/18884 migrate avatar network #19079

thebinij · 2023-05-09T16:27:43Z

Explanation

Closes Migrate components to TS: AvatarNetwork #18884

Screenshots/Screencaps

Before

before.mp4

After

after.mp4

Manual Testing Steps

No functional changes

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

github-actions · 2023-05-09T16:27:56Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

thebinij · 2023-05-09T17:17:43Z

Hi @georgewrmarshall, I have updated the code after you suggestions in the Old PR. Please review it. I need your help to understand causation of error in test-deps-depcheck. Thanks

georgewrmarshall

Looking good. Left one suggestion that I missed in my last review. And it will need a rebase

georgewrmarshall · 2023-05-14T04:32:46Z

ui/components/component-library/avatar-network/avatar-network.types.ts

+/**
+ * Props for the AvatarNetwork component
+ */
+export interface AvatarNetworkProps extends BoxProps {


I think we could extend AvatarBaseProps here instead of BoxProps and remove backgroundColor, borderColor, color, as types altogether?

Yeah, that would be great.

georgewrmarshall · 2023-05-14T04:35:24Z

ui/components/component-library/avatar-base/avatar-base.types.ts

@@ -20,7 +21,7 @@ export interface AvatarBaseProps extends TextProps {
   * 'AvatarBaseSize.Md'(32px), 'AvatarBaseSize.Lg'(40px), 'AvatarBaseSize.Xl'(48px)
   * Defaults to AvatarBaseSize.Md
   */
-  size?: AvatarBaseSize;
+  size?: AvatarBaseSize | AvatarNetworkSize;


Not sure about this because if we carry on this pattern we will just end up with many variant types that AvatarBase accepts. There must be a TypeScript pattern that uses an argument type? I'm willing to proceed until we develop that pattern

codecov · 2023-05-14T08:18:44Z

Codecov Report

Merging #19079 (763dc3d) into develop (e923110) will decrease coverage by 0.01%.
The diff coverage is 95.65%.

❗ Current head 763dc3d differs from pull request most recent head 6a0732f. Consider uploading reports for the commit 6a0732f to get more accurate results

@@             Coverage Diff             @@
##           develop   #19079      +/-   ##
===========================================
- Coverage    69.35%   69.34%   -0.01%     
===========================================
  Files          986      985       -1     
  Lines        37265    37263       -2     
  Branches     10002    10003       +1     
===========================================
- Hits         25843    25839       -4     
- Misses       11422    11424       +2

Impacted Files	Coverage Δ
ui/components/app/add-network/add-network.js	`56.25% <ø> (ø)`
...list-item/smart-transaction-list-item.component.js	`3.12% <ø> (ø)`
...ction-list-item/transaction-list-item.component.js	`70.27% <ø> (ø)`
...ents/component-library/avatar-base/avatar-base.tsx	`100.00% <ø> (ø)`
...component-library/picker-network/picker-network.js	`100.00% <ø> (ø)`
ui/components/multichain/nft-item/nft-item.js	`100.00% <ø> (ø)`
...ents/multichain/token-list-item/token-list-item.js	`100.00% <ø> (ø)`
...omponent-library/avatar-network/avatar-network.tsx	`95.65% <95.65%> (ø)`

... and 2 files with indirect coverage changes

georgewrmarshall

Hey @thebinij, one more suggestion

georgewrmarshall · 2023-05-18T00:15:47Z

ui/components/component-library/avatar-network/avatar-network.types.ts

+  /**
+   * The name accepts the string to render the first alphabet of the Avatar Name
+   */
+  name?: string;


I think we should make this required so the AvatarNetwork always has a child

Okay, I have made "name" to be required. And also omitted "children" from the Base like suggested in AvatarIcon's PR

georgewrmarshall

Hey @thebinij, code is looking good could you please add a screencast from storybook of the component docs and stories to the PR description. That way we can ensure there is no visual regression and everything works as expected. Thanks!

thebinij · 2023-05-19T10:41:39Z

Hey @thebinij, code is looking good could you please add a screencast from storybook of the component docs and stories to the PR description. That way we can ensure there is no visual regression and everything works as expected. Thanks!

Hi @georgewrmarshall, I have included the before and after screencast to the description.

georgewrmarshall · 2023-05-31T22:19:52Z

Hey @thebinij, Thanks for your time and effort in creating this pull request. As you're probably aware we have identified an issue related to the typing of the Box component, specifically with the polymorphic as prop. To address this issue, we have an open ticket at #19239 and a draft pull request at #19363 that needs to be merged before we can proceed with your PR. Thanks for your patience

thebinij · 2023-06-25T06:00:27Z

Hi @georgewrmarshall, Is there anything I can update in the PR before so you approve to merge it?

georgewrmarshall · 2023-06-26T21:09:40Z

Blocked by #19572

fixing error fix textAlign added AvatarNetworkSize NetworkProps extends BaseProps instead of Boxprops omitted children from base, made name required replace deprecated and fix lint

garrettbear · 2023-07-20T21:59:49Z

@thebinij jumped in to help you out with some of the recent updates made the Box component and AvatarBase

garrettbear · 2023-07-20T22:01:44Z

ui/components/component-library/avatar-base/avatar-base.types.ts

@@ -14,7 +14,7 @@ export enum AvatarBaseSize {
  Xl = 'xl',
 }

-export interface Props extends TextStyleUtilityProps {
+export interface AvatarBaseStyleUtilityProps extends TextStyleUtilityProps {


@georgewrmarshall renamed the base props here to be consistent with how we have been doing it lately.

ui/components/component-library/avatar-network/README.mdx

georgewrmarshall

Unfortunately we can't reassign enums you can sort of see what happens when using the storybook controls

Also not sure why the props table isn't showing up?

It would be helpful to update the Before/After screencasts to ensure the storybook still works
https://metamask.github.io/metamask-storybook/?path=/docs/components-componentlibrary-avatarnetwork--docs

ui/components/component-library/avatar-network/avatar-network.types.ts

…types.ts Co-authored-by: George Marshall <[email protected]>

georgewrmarshall

Looking good! One suggestion to remove the JS version of the Box from avatar-network.stories.tsx

And can we update this line in the ui/components/component-library/avatar-network/README.mdx to point to the TS Box props

The `AvatarNetwork` accepts all props below as well as all [Box](/docs/components-ui-box--default-story#props) component props

to

The `AvatarNetwork` accepts all props below as well as all [Box](/docs/components-componentlibrary-box--docs#props) component props

ui/components/component-library/avatar-network/avatar-network.stories.tsx

georgewrmarshall

LGTM! Thanks for your contribution @thebinij and supporting @garrettbear

thebinij requested a review from a team as a code owner May 9, 2023 16:27

thebinij requested a review from legobeat May 9, 2023 16:27

georgewrmarshall requested review from georgewrmarshall and garrettbear May 10, 2023 00:22

georgewrmarshall added the team-design-system All issues relating to design system in Extension label May 10, 2023

georgewrmarshall reviewed May 14, 2023

View reviewed changes

georgewrmarshall reviewed May 18, 2023

View reviewed changes

georgewrmarshall reviewed May 19, 2023

View reviewed changes

thebinij force-pushed the fix/18884-Migrate-AvatarNetwork branch 2 times, most recently from 85379a1 to a962795 Compare May 29, 2023 06:55

thebinij force-pushed the fix/18884-Migrate-AvatarNetwork branch from a962795 to f87390c Compare June 13, 2023 17:02

thebinij force-pushed the fix/18884-Migrate-AvatarNetwork branch from f87390c to 5ec399f Compare June 25, 2023 06:00

Migrate AvatarNetwokr

724efad

fixing error fix textAlign added AvatarNetworkSize NetworkProps extends BaseProps instead of Boxprops omitted children from base, made name required replace deprecated and fix lint

garrettbear force-pushed the fix/18884-Migrate-AvatarNetwork branch from 5ec399f to 724efad Compare July 20, 2023 20:54

update AvatarNetwork TS

36f9718

garrettbear added the needs-ux-ds-review label Jul 20, 2023

garrettbear reviewed Jul 20, 2023

View reviewed changes

add AvatarNetworkSize test

fa043c3

garrettbear requested review from garrettbear and georgewrmarshall July 20, 2023 22:13

garrettbear added 2 commits July 21, 2023 10:13

remove unused size import

82cdb05

Merge branch 'develop' into fix/18884-Migrate-AvatarNetwork

9be61ea

georgewrmarshall reviewed Jul 21, 2023

View reviewed changes

ui/components/component-library/avatar-network/README.mdx Show resolved Hide resolved

georgewrmarshall requested changes Jul 21, 2023

View reviewed changes

ui/components/component-library/avatar-network/avatar-network.types.ts Outdated Show resolved Hide resolved

garrettbear and others added 2 commits July 21, 2023 14:26

Update ui/components/component-library/avatar-network/avatar-network.…

c829c6b

…types.ts Co-authored-by: George Marshall <[email protected]>

fix readme

763dc3d

garrettbear requested a review from georgewrmarshall July 21, 2023 21:59

georgewrmarshall requested changes Jul 24, 2023

View reviewed changes

ui/components/component-library/avatar-network/avatar-network.stories.tsx Outdated Show resolved Hide resolved

update to latest box component

6a0732f

georgewrmarshall approved these changes Jul 24, 2023

View reviewed changes

pedronfigueiredo approved these changes Jul 24, 2023

View reviewed changes

garrettbear merged commit ceadfac into MetaMask:develop Jul 24, 2023

github-actions bot locked and limited conversation to collaborators Jul 24, 2023

georgewrmarshall added the external-contributor label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/18884 migrate avatar network #19079

Fix/18884 migrate avatar network #19079

thebinij commented May 9, 2023 •

edited

Loading

github-actions bot commented May 9, 2023

thebinij commented May 9, 2023

georgewrmarshall left a comment

georgewrmarshall May 14, 2023

thebinij May 14, 2023

georgewrmarshall May 14, 2023

codecov bot commented May 14, 2023 •

edited

Loading

georgewrmarshall left a comment

georgewrmarshall May 18, 2023

thebinij May 18, 2023

georgewrmarshall left a comment •

edited

Loading

thebinij commented May 19, 2023

georgewrmarshall commented May 31, 2023

thebinij commented Jun 25, 2023

georgewrmarshall commented Jun 26, 2023

garrettbear commented Jul 20, 2023

garrettbear Jul 20, 2023

georgewrmarshall left a comment

georgewrmarshall left a comment •

edited

Loading

georgewrmarshall left a comment •

edited

Loading

Fix/18884 migrate avatar network #19079

Fix/18884 migrate avatar network #19079

Conversation

thebinij commented May 9, 2023 • edited Loading

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented May 9, 2023

thebinij commented May 9, 2023

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall May 14, 2023

Choose a reason for hiding this comment

thebinij May 14, 2023

Choose a reason for hiding this comment

georgewrmarshall May 14, 2023

Choose a reason for hiding this comment

codecov bot commented May 14, 2023 • edited Loading

Codecov Report

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall May 18, 2023

Choose a reason for hiding this comment

thebinij May 18, 2023

Choose a reason for hiding this comment

georgewrmarshall left a comment • edited Loading

Choose a reason for hiding this comment

thebinij commented May 19, 2023

georgewrmarshall commented May 31, 2023

thebinij commented Jun 25, 2023

georgewrmarshall commented Jun 26, 2023

garrettbear commented Jul 20, 2023

garrettbear Jul 20, 2023

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment • edited Loading

Choose a reason for hiding this comment

georgewrmarshall left a comment • edited Loading

Choose a reason for hiding this comment

thebinij commented May 9, 2023 •

edited

Loading

codecov bot commented May 14, 2023 •

edited

Loading

georgewrmarshall left a comment •

edited

Loading

georgewrmarshall left a comment •

edited

Loading

georgewrmarshall left a comment •

edited

Loading